Is is not valid for empty string views to have nullptr data pointers?

It is valid, though we'd need to be careful with hashing etc. Anyhow, this empty string does not need any storage, so it seems simpler to just skip it (might also be faster but I didn't measure).

But if someone tries to intern the string_view {nullptr, 0} and we create a View at nullptr, then trying to turn that View back into a string_view will segfault. That seems bad, right?

Can't a std::string_view be null (internal pointer is nullptr)? Where would it segfault?

Where it tries to dereference the pointer in the View (actually 4 bytes before it) to get the size.

Oh right, View needs to special-case a null to avoid a read. I also see that passing nullptr, 0 is not defined behavior anyhow. Fixed.

Interned empty string views can have str.internal == nullptr, but now that is interpreted as the absence of a name rather than the presence of an empty name. I don't think this is the intended behavior.

An empty name is non-null internal pointing to some place, and that place has a size of 0? I think this should be ok. Pushed a unit test now, did I do something wrong there?

We also need this:

auto default = IString(std::string_view{});

This should be the same as empty, but I think it will currently be the same as null.

Hmm, why should that be empty? I believe the default std::string_view is a null. The test I just added confirms that unless I'm confused.

Well std::string_view{} == std::string_view("") holds, so the interned versions of those should be the same as well.

Hmm, supporting that would add overhead. Do you think it's worth it?

There shouldn't be any extra overhead except the first time we intern the empty stringref. We just need to remove the early return I commented on here and let the default empty string_view get interned like any other string_view.

That almost works but we also need to handle the case of comparing that interned string to an IString(), which never had anything interned for it, and just contains nullptr. That requires either

1. Making IString() (the default value throughout the codebase) call interned() to intern.
2. Make operator== return "equal" if both sizes are 0, no matter what data they point to.

I'd rather not add either overhead. I don't think we must follow std::string_view's API. (Instead, we would be following other languages that do differentiate the null string from the empty string, so it's not like I'm suggesting some chaotic behavior 😄 )

But the interned empty string_view won't have nullptr data, so it won't be confused with a default IString, which will have nullptr data, right? It's expected that IString() is not equal to IString(std::string_view{}) because the latter should be equal to IString("").

Oh, maybe I misunderstood you before then. When you said

Well std::string_view{} == std::string_view("") holds, so the interned versions of those should be the same as well.

Did you mean "IF we interned the null string it should compare equal to the empty string (BUT we don't so things are ok as is?)"

If not, then I am confused. What concrete change to the API are you asking for?

(Note that I added a test early in this thread - is that test wrong in your opinion? What should the test actually check at the API level?)

I would expect these test expectations to pass both before and after this refactoring:

ASSERT_EQ(IString(""), IString(std::string_view{})); // !
ASSERT_NE(IString(), IString(""));
ASSERT_NE(IString(), IString(std::string_view{})); // !
ASSERT_FALSE(IString().is());
ASSERT_TRUE(IString("").is());
ASSERT_TRUE(IString(std::string_view{}).is()); // !

My understanding of the current change is that the assertions involving the default string_view will fail. Since this change is supposed to be NFC, this is a bug.